🔍 Rust PR Review

Summary: The design and implementation are solid, but there is one logic bug in the CI trigger narrowing that would silently break the feature for the typical PR workflow (feature branch → PR → main), plus a minor injection surface to verify.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs generate_ci_trigger — narrowed trigger: uses target branches, not source branches

  The PR's synthetic-from-ci mechanism works by:

  1. A developer pushes to feature/x → ADO fires CI for feature/x (Build.SourceBranch = refs/heads/feature/x)
  2. exec-context-pr-synth.js fetches active PRs whose sourceRefName = refs/heads/feature/x, filters by targetRefName matching pr.branches.include
  3. On a match, the build is promoted to PR semantics

  For step 1 to happen, the compiled pipeline's trigger: block must fire for feature/x. But generate_ci_trigger emits:

  trigger:
    branches:
      include:
        - 'main'    # pr.branches.include — these are PR TARGET branches

  ADO trigger: fires on pushes to the listed branches. With include: [main], only pushes to main trigger the pipeline. Pushes to feature/x are suppressed, so exec-context-pr-synth.js never runs on those builds.

  The test harness confirms the intended runtime contract: BUILD_SOURCEBRANCH = refs/heads/feature/x is the default in harness.ts::makeEnv, and the index tests simulate CI builds on feature branches. But those CI builds will never occur if the compiled trigger only includes target branches.

  Fix: Remove the trigger narrowing in branch 2 of generate_ci_trigger. Without an explicit trigger: block, ADO defaults to all-branches CI, which is what synthPr needs. The AW_SYNTHETIC_PR_SKIP fast-exit in the Setup job already handles wasted builds cheaply (one listActivePullRequestsBySourceRef call that returns [] for branches with no PRs). Alternatively, the narrowing should be documented as an opt-in trade-off where users accept that only direct-to-target-branch pushes get synthPr processing.

  The five generate_ci_trigger tests and the synthetic-pr-default.md snapshot fixture all pass because they test the emitted YAML, not the runtime effect of narrowing on feature branch builds.

🔒 Security Concerns

• scripts/ado-script/src/exec-context-pr-synth/index.ts:163-167 — setOutput with API-derived branch names

  setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? "");
  setOutput("AW_SYNTHETIC_PR_SOURCEBRANCH", pr.sourceRefName ?? sourceBranch);

  pr.targetRefName and pr.sourceRefName come directly from the ADO REST API. If setOutput in vso-logger.ts doesn't sanitise values for ##vso[...] command injection, a repository owner who controls branch names could craft refs/heads/main##vso[task.setSecret]secretValue to inject logging commands. This is the same class of risk the project already guards against for addBuildTag's colon (fix(gate): use '.' separator in build tags so ADO doesn't reject ':' in REST path #917).

  ADO branch naming rules in practice make real-world exploitation unlikely, but it's worth confirming that vso-logger.ts::setOutput applies the same logInfo-style sanitisation used for bypass_label in bypass.ts, or add explicit value sanitisation here. (Low priority.)

✅ What Looks Good

• Spec validation is hardened in the right direction: decodeSpec in spec.ts hard-fails on any malformed PR_SYNTH_SPEC rather than soft-skipping — correct, since treating spec corruption as a skip would silently widen the synth attack surface.
• build_pr_synth_spec size cap matches the GATE_SPEC ceiling (8 KiB); the oversize test with 1000 branch globs is a solid regression guard.
• Coalesce expressions make real PR builds bit-identical: when synthPr step doesn't run (Build.Reason = PullRequest), coalesce(null, '') returns '', leaving real System.PullRequest.* variables in play.
• synthetic_pr_active is true but pr_trigger_for_synth is None invariant check in AdoScriptExtension::setup_steps — good defensive programming for the compile-time structural contract.
• Base64-encoded PR_SYNTH_SPEC is safe to embed in a YAML double-quoted scalar (base64 alphabet [A-Za-z0-9+/=] has no YAML double-quoted special characters).
• AW_SYNTHETIC_PR_SKIP guard placement in generate_agentic_depends_on is correct: it's evaluated first (ne(synthPr.AW_SYNTHETIC_PR_SKIP, 'true') before the or(Build.Reason, ...) clause) ensuring a synth-skip decision is always honoured over other activation reasons.
• Step-level condition on synthPr step (ne(Build.Reason, 'PullRequest')) is the right defence-in-depth: even if the downstream coalesce expressions were wrong, the step would never execute on a real PR build.
• Test coverage is comprehensive: round-trip spec tests, runtime contract tests with mocked ADO client (real-PR no-op, GitHub no-op, zero/multi-match skips, path-filter rejection, happy path, draft PR), snapshot fixtures with substring-negation back-compat guard.

Generated by Rust PR Reviewer for issue #922 · sonnet46 3.4M · ◷

🔍 Rust PR Review

Summary: Needs changes — good architecture overall, but one logic regression and one misleading doc fragment.

Findings

🐛 Bugs / Logic Issues

• src/compile/common.rs:2409-2417 — filter gate bypassed for real PR builds when synthetic-from-ci: true

  When synthetic_pr_active=true (the default for any on.pr: config) and has_pr_filters=true, the new agent-job dependsOn condition is:

  or(
    eq(variables['Build.Reason'], 'PullRequest'),    // arm a — unconditional for real PRs
    eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'),   // arm b
    eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')          // arm c
  )
  

  Arm (a) is unconditionally true on every real PR build. This is a regression from the original condition:

  or(
    ne(variables['Build.Reason'], 'PullRequest'),  // non-PR always passes
    eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')  // PR requires gate
  )
  

  The original required SHOULD_RUN=true for real PR builds; the new condition does not.

  When it bites: the gate soft-skips when SYSTEM_ACCESSTOKEN is unavailable — it sets SHOULD_RUN=false and exits 0 rather than cancelling the build. In that scenario a real PR with a mismatching title/label/draft filter will have succeeded()=true and arm(a)=true, so the agent runs despite the filter saying no. Since synthetic-from-ci defaults to true, this affects all existing agents with on.pr + filters:.

  Suggested fix — require the gate to pass in both PR paths:

  // synthetic_pr_active && has_pr_filters
  parts.push(
      r"and(
       or(
         eq(variables['Build.Reason'], 'PullRequest'),
         eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true')
       ),
       eq(dependencies.Setup.outputs['prGate.SHOULD_RUN'], 'true')
     )"
      .to_string(),
  );

  This preserves "run if (real PR or synthetic) AND gate passed" rather than treating either status as an unconditional pass.

📄 Documentation Bug

• prompts/update-ado-agentic-workflow.md:51 — incorrect claim:

  auto-narrows the top-level trigger: block to those branches.

  The code does not do this — there are tests (test_generate_ci_trigger_synthetic_pr_does_not_narrow) and a multi-paragraph design note in docs/front-matter.md explaining exactly why CI-trigger narrowing was removed as a logic bug. This sentence is a leftover from an earlier iteration. The create-ado-agentic-workflow.md prompt got this right; update-ado-agentic-workflow.md didn't.

⚠️ Suggestions

• scripts/ado-script/src/shared/ado-client.ts:106 — comment says "first page (200 PRs)" but getPullRequests is called without a top argument; the azure-devops-node-api default is 100. Behaviour is correct (exactly-one contract catches pathological cases anyway), but the comment is misleading.

✅ What Looks Good

• bypass.ts: the synthetic guard requires both spec.context.build_reason === "PullRequest" and AW_SYNTHETIC_PR === "true", so a stray env var cannot suppress bypass on a pipeline-completion gate spec.
• spec.ts treats decode corruption as a hard error rather than a soft skip — correct, since silently widening the matched-branch set would be a security gap.
• synthetic_pr_step() has a step-level ne(Build.Reason, 'PullRequest') condition so real PR builds see zero overhead.
• PrTriggerConfig uses a hand-written Default impl delegating to pr_synthetic_from_ci_default() — avoids the #[derive(Default)] footgun where a non-false default becomes false.
• 8 KiB cap on PR_SYNTH_SPEC mirrors GATE_SPEC — good defence-in-depth against pathological front-matter.
• match.ts strips the refs/heads/ prefix before picomatch comparison, so main and refs/heads/main both work in on.pr.branches.include.

Generated by Rust PR Reviewer for issue #922 · sonnet46 3.6M · ◷

🔍 Rust PR Review

Summary: Ambitious and well-designed feature — architecture is clean, test coverage is strong, but there are two runtime correctness concerns about how ADO evaluates the synthesised env-block expressions that should be verified before merge.

Findings

🐛 Bugs / Logic Issues

1. Gate step uses a cross-job output reference within the same job (src/compile/filter_ir.rs, compile_gate_step_external)

The gate step is emitted into the Setup job (returned from AdoScriptExtension::setup_steps). When synthetic_pr_active = true, the env block gains:

AW_SYNTHETIC_PR: $[ coalesce(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], '') ]
ADO_PR_ID: $[ coalesce(variables['System.PullRequest.PullRequestId'], dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID']) ]

dependencies.Setup.outputs['synthPr.X'] is cross-job syntax — it resolves correctly in downstream jobs (the Agent job), but within the Setup job itself the synthPr step's isOutput=true variables are accessible as $(synthPr.AW_SYNTHETIC_PR) (macro) or $[ steps.synthPr.outputs['AW_SYNTHETIC_PR'] ] (runtime expression). The dependencies.JobName.outputs[...] form is undefined inside the producing job.

Practical consequence: bypass.ts checks process.env.AW_SYNTHETIC_PR === "true". If the coalesce cannot resolve the synthPr output, AW_SYNTHETIC_PR arrives empty and bypass takes the "not a PR build → auto-pass" path for CI-promoted builds — the gate skips, defeating the PR filter entirely.

2. $[ coalesce(...) ] in step-level env: blocks (filter_ir.rs, exec_context/pr.rs)

ADO documents $[...] runtime expressions as valid in variables: blocks and condition: fields, but step-level env: blocks use $(...) macro expansion. If ADO treats $[ coalesce(...) ] as a literal string in an env: value, all downstream coalescing silently fails: SYSTEM_PULLREQUEST_PULLREQUESTID, ADO_PR_ID, etc. arrive at the bundle as the raw expression string rather than the actual PR identifier.

The standard pattern for accessing cross-job outputs in a step env is to map the output to a job-level variable first:

# Agent job
variables:
  synthPrId: $[ dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR_ID'] ]
steps:
  - bash: ...
    env:
      ADO_PR_ID: $(synthPrId)   # macro, not $[]

This would require a compiler-generated variables: block per job. Worth an explicit ADO YAML reference or empirical test before merge, given the deferred E2E.

3. AW_SYNTHETIC_PR_TARGETBRANCH format inconsistency (exec-context-pr-synth/index.ts)

setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", pr.targetRefName ?? "") stores refs/heads/main (ADO API format). But System.PullRequest.TargetBranch predefined variable is main (no prefix). After coalescing, exec-context-pr.js sees main for real PR builds and refs/heads/main for synth-promoted ones. If the bundle uses this for checkout or merge-base resolution, synth builds break. Fix: strip the prefix before setOutput:

setOutput("AW_SYNTHETIC_PR_TARGETBRANCH", (pr.targetRefName ?? "").replace(/^refs\/heads\//, ""));

⚠️ Suggestions

4. Gate enforcement silently bypassed for real PR builds in synthetic mode (src/compile/common.rs, generate_agentic_depends_on)

With synthetic_pr_active=true + has_pr_filters=true, the agent-job condition includes eq(Build.Reason, 'PullRequest') as a standalone OR arm. A real PR build that fails the gate (prGate.SHOULD_RUN=false, e.g., title doesn't match *[review]*) still passes this arm and runs the agent. Pre-synthetic mode enforced the gate for real PR builds. If intentional for synth mode, a comment explaining the design decision would prevent confusion for authors upgrading existing on.pr.filters: agents (default is now mode: synthetic).

5. Stale compile-coalesce-env todo comment (src/compile/extensions/exec_context/pr.rs ~line 133)

"The step's condition is also broadened in the compile-coalesce-env todo" — the condition broadening is already implemented in the same function. Should be removed or updated.

✅ What Looks Good

• Design: PrMode enum, PR_SYNTH_SPEC shape and 8 KiB size cap, exactly-one-match rule, and the comment explaining why the CI trigger must NOT be auto-narrowed to pr.branches.include (target vs source branch inversion) are all well-reasoned.
• Test coverage: 26 new vitest cases, 4 new generate_ci_trigger Rust unit tests, round-trip spec tests, and 2 new snapshot fixtures are thorough. The assertions specifically checking no auto-narrowed CI trigger are valuable regression guards.
• Bypass guard scoping: bypass.ts correctly limits the synthetic-PR suppression to PullRequest specs only; the four new bypass tests cover all edge cases cleanly.
• Security posture: Treating PR_SYNTH_SPEC decode failures as hard errors (exit 1, not soft skip) is the right call — a malformed spec should never widen the match surface silently.

Generated by Rust PR Reviewer for issue #922 · sonnet46 4.9M · ◷

🔍 Rust PR Review

Summary: Looks good overall — well-structured feature with strong test coverage and clear security reasoning. A few items worth fixing.

Findings

🐛 Bugs / Logic Issues

• exec-context-pr-synth/index.ts, runtime contract comment (line ~443): The comment says 4. branches.include/exclude miss on BUILD_SOURCEBRANCH → skip, but the code never filters on.pr.branches against BUILD_SOURCEBRANCH. It fetches PRs by sourceRefName == sourceBranch, then filters the matched PRs by their targetRefName against spec.branches. This is correct ADO semantics (pr.branches = target branches), but the contract comment is wrong and could mislead a future debugger into thinking the spec's branch list is being applied to the feature branch itself.

• ado-client.ts comment (line ~937): Says "first page (200 PRs)" — ADO's getPullRequests default page size without top is 100, not 200.

⚠️ Suggestions

• AdoScriptExtension.synthetic_pr_active + pr_trigger_for_synth coupling: These two fields must be set together (true ↔ Some(...)), enforced only by a runtime anyhow::anyhow! guard in setup_steps. The error message is clear and tests would catch a mismatch quickly, but wrapping them in a single Option<PrTriggerConfig> (using is_some() as the activation predicate) would make the invariant unrepresentable-wrong at the type level. Low urgency since the existing tests cover the pairing.

• $[...] runtime expressions as unquoted YAML scalars (filter_ir.rs, exec_context/pr.rs): Values like $[ coalesce(variables['synthPr.AW_SYNTHETIC_PR'], '') ] are embedded as unquoted block-mapping scalars containing single quotes. ADO's YAML parser handles these fine in practice and the existing gate-step YAML does the same, but the strict YAML spec reserves ' as a scalar indicator at the start of a value. Wrapping these in YAML double quotes (e.g. "$[ coalesce(...) ]") would be strictly correct and is the form shown in ADO documentation. Not a functional bug with ADO.

✅ What Looks Good

• Gate-bypass security fix is solid: The AND-NOT form and(ne(Build.Reason,'PullRequest'), ne(synthPr.AW_SYNTHETIC_PR,'true')) as the "unconditional pass" arm is exactly right. An earlier iteration with standalone eq(...) bypass arms would have let any PR build skip the gate — this version forces gate evaluation for all PR-flavour builds. The regression guards in the tests are excellent.
• Same-job vs cross-job variable reference distinction is correctly handled and explicitly documented in both filter_ir.rs and exec_context/pr.rs.
• Base64 spec injection safety: base64 alphabet contains no YAML-special characters; embedding in a double-quoted YAML scalar is safe.
• PrMode::Synthetic as #[default] is the right choice — zero migration friction for existing on.pr agents, and the opt-out path (mode: policy) is explicit.
• Test coverage is thorough: the regression guard for the gate-bypass bug (test_pr_filter_synth_mode_agent_condition_enforces_gate) and the flipped assertion in test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup both have good explanatory comments.

Generated by Rust PR Reviewer for issue #922 · sonnet46 2.5M · ◷

🔍 Rust PR Review

Summary: Looks good — solid design with a few minor observations worth addressing before merge.

Findings

⚠️ Suggestions

• scripts/ado-script/src/shared/ado-client.ts:117-127 — Missing null guard on listActivePullRequestsBySourceRef

  The azure-devops-node-api SDK's getPullRequests may return null instead of [] when no PRs match (the SDK uses null for empty bodies in some REST responses). Calling .filter(...) on null at index.ts:103 would throw at runtime. The pattern is already established in this file — getIterationChanges uses result.changeEntries ?? []. A one-liner guard is all it takes:

  // ado-client.ts
  return (await git.getPullRequests(
    repoId,
    { sourceRefName, status: PullRequestStatus.Active },
    project,
  )) ?? [];

• src/compile/extensions/exec_context/pr.rs:121-178 — condition: string is non-trivially long and untested for the synth+no-synth branches

  The condition string inside the format is embedded verbatim into the condition: YAML field. When synthetic_pr_active is true, the condition is:

  or(eq(variables['Build.Reason'], 'PullRequest'), eq(dependencies.Setup.outputs['synthPr.AW_SYNTHETIC_PR'], 'true'))
  

  This is only tested indirectly via the snapshot fixtures. A targeted unit test verifying both the synth and non-synth step outputs would help guard against accidental regressions here, similar to how the gate-step same-job vs cross-job distinction is tested.

• src/compile/types.rs — SanitizeConfigTrait impl for PrTriggerConfig doesn't mention mode

  The new mode: PrMode field is a Copy enum with no string content, so there's nothing to sanitize. Still worth a one-line comment in the sanitize_config_fields impl explaining why mode is intentionally absent:

  fn sanitize_config_fields(&mut self) {
      // mode (PrMode enum) has no string content to sanitize.
      if let Some(ref mut b) = self.branches { ... }

✅ What Looks Good

• Same-job vs cross-job reference distinction is well-handled. The gate step (Setup job) correctly uses variables['synthPr.X'] while the Agent-job dependsOn condition uses dependencies.Setup.outputs['synthPr.X']. This is clearly documented in the function doc-comments and the regression tests for both forms (test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref) are excellent.

• AdoScriptExtension::pr_trigger_for_synth: Option<...> as single source of truth. Encoding "synth active" in the Option type rather than a separate bool eliminates an invariant-coordination bug class. Clean design.

• Gate bypass change in bypass.ts. The spec.context.build_reason === "PullRequest" && AW_SYNTHETIC_PR === "true" guard is correctly scoped to PR gates only — pipeline-completion gates are unaffected. No way for a non-PR synthetic flag to mistakenly disable the bypass for unrelated gate types.

• spec.ts validation is correctly strict. Spec corruption is a hard failure (return 1), not a soft skip. This is the right call — silently skipping on a tampered spec would widen the attack surface. The comment explaining this in the source is good.

• generate_agentic_depends_on's gate enforcement for synth mode is correct. The AND-NOT clause ne(Build.Reason, 'PullRequest') AND ne(synthPr.AW_SYNTHETIC_PR, 'true') correctly prevents the gate being bypassed for either real PR builds or synth-promoted CI builds. The regression test test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_gate_enforced_pr_clause with its defensive "must NOT contain the buggy bypass arms" assertions is exactly the right guard here.

• build_pr_synth_spec 8 KiB cap mirrors the existing GATE_SPEC ceiling. Consistent defence-in-depth.

• Sanitize is called before collect_extensions / compile_shared (in mod.rs:150), so branch/path globs embedded in PR_SYNTH_SPEC are already sanitized. No injection window.

Generated by Rust PR Reviewer for issue #922 · sonnet46 3.9M · ◷

🔍 Rust PR Review

Summary: Solid, well-structured implementation — looks good with a few small items worth addressing.

Findings

🐛 Bugs / Logic Issues

• tests/compiler_tests.rs:4251 — Stale test name contradicts the assertion it contains.
  test_exec_context_pr_only_downloads_bundle_in_agent_job_not_setup was renamed in intent but not in name: the test now asserts setup.contains("Download ado-aw scripts") (the download IS expected in Setup), but the function name says "not setup". It passes, but the next engineer reading the failure message will be confused. Should be renamed to something like test_exec_context_pr_downloads_bundle_in_both_jobs_with_synth_mode.

⚠️ Suggestions

• synthetic_pr_active predicate duplicated in three places.
  The expression front_matter.pr_trigger().is_some_and(|p| matches!(p.mode, PrMode::Synthetic)) appears independently in compile_shared (common.rs ~line 3374), ExecContextExtension::new (exec_context/mod.rs ~line 128), and collect_extensions (extensions/mod.rs ~line 703). If a third PrMode variant is ever added the activation logic must be updated in all three spots. A FrontMatter::is_synthetic_pr() -> bool helper would be the idiomatic fix.

• src/compile/types.rs — Truncated doc comment on PrTriggerConfig.mode.
  /// Whether to synthesise PullRequest semantics on CI builds when an / /// PR-trigger mode. reads as a cut-off sentence. The "when an PR-trigger mode" fragment has no predicate. Suggest rewriting to something like: /// Determines how PR builds reach the pipeline; see [PrMode].

✅ What Looks Good

• Same-job vs. cross-job ADO variable reference is handled correctly and heavily documented: the gate step (inside Setup) uses variables['synthPr.X']; the Agent-job condition and exec-context-pr step use dependencies.Setup.outputs['synthPr.X']. Two dedicated regression tests (test_pr_filter_synth_mode_gate_step_uses_same_job_synth_ref, prepare_step_synth_active_emits_coalesced_env_and_broadened_condition) pin both forms.
• Gate-bypass security invariant is correctly enforced: the Agent-job condition requires the gate to pass for both real-PR and synth-promoted builds (eq(prGate.SHOULD_RUN, 'true')), with no standalone OR arm that could let a PR build bypass the filter. The defensive regression test (test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_gate_enforced_pr_clause) explicitly asserts the "buggy bypass arms" are absent.
• PrMode as a closed enum — invalid front-matter values are rejected at deserialization time, the SanitizeConfigTrait impl correctly explains no sanitization is needed, and the test_pr_trigger_config_mode_invalid_value_errors test pins this.
• PR_SYNTH_SPEC base64-encoding is safe for YAML embedding (base64 alphabet has no YAML-special characters), the 8 KiB cap matches the GATE_SPEC precedent, and oversize rejection is tested.
• Step ordering (synthPr → prGate) is enforced in code and pinned by setup_steps_emits_synth_step_before_gate_when_both_active.
• bypass.ts change is minimal and correctly scoped — AW_SYNTHETIC_PR suppresses the bypass only when spec.context.build_reason === "PullRequest", so pipeline-completion specs are unaffected.

Generated by Rust PR Reviewer for issue #922 · sonnet46 2.5M · ◷

🔍 Rust PR Review

Summary: Looks good — well-designed feature with strong correctness properties and comprehensive tests. Two minor suggestions below.

Findings

⚠️ Suggestions

• src/compile/extensions/ado_script.rs:57–92 — Two impl AdoScriptExtension blocks now exist back-to-back: one added for synthetic_pr_active(), and the pre-existing one for lowered_checks(). Both are valid Rust, but they could be merged to reduce noise. Low priority.

• scripts/ado-script/src/exec-context-pr-synth/match.ts:47,62 — picomatch(g, { dot: true }) is called inside .some(), creating a fresh compiled matcher on every (pattern, value) pair. The function is called once per build so this has zero practical impact, but the idiomatic form would compile each pattern once:

  const matchers = normIncludes.map(g => picomatch(g, { dot: true }));
  const includeMatches = normIncludes.length === 0 || matchers.some(m => m(normalised));

✅ What Looks Good

• Same-job vs. cross-job ADO reference distinction is correctly handled: the gate step (lives in Setup job) uses variables['synthPr.X'], while the exec-context-pr step and Agent-job dependsOn condition (live downstream of Setup) use dependencies.Setup.outputs['synthPr.X']. This is the most subtle correctness invariant in the PR and it's right everywhere, with clear doc-comments explaining why.

• Gate bypass enforced for both real-PR and synth-promoted builds: the generate_agentic_depends_on condition correctly uses the AND-NOT form (ne(Build.Reason,'PullRequest') AND ne(synthPr.AW_SYNTHETIC_PR,'true')) rather than permissive OR-arms that would silently bypass the gate. The corresponding regression guard test (test_agentic_depends_on_synthetic_pr_active_emits_skip_guard_and_gate_enforced_pr_clause) is thorough.

• is_synthetic_pr() centralization: the single predicate in FrontMatter eliminates the risk of the three call-sites drifting if a future PrMode variant is added. The type-level encoding via Option<PrTriggerConfig> (present ↔ active) is a nice touch in AdoScriptExtension.

• Hard vs. soft failure contract in exec-context-pr-synth/index.ts: spec decode errors and infra failures exit non-zero (step failure); no-match / multi-match cases emit AW_SYNTHETIC_PR_SKIP=true and exit 0. This keeps builds green and prevents noise for the common "CI push with no open PR" case.

• PR_SYNTH_SPEC security boundary: user-supplied globs are serialised via serde_json (properly escaping any special characters), base64-encoded, and validated by the TypeScript bundle's decodeSpec → validate chain before any pattern matching. The 8 KiB cap mirrors the GATE_SPEC ceiling.

• CI trigger non-narrowing rationale is documented in three places (code comment in generate_ci_trigger, docs/front-matter.md, and the test assertion message) — future maintainers won't accidentally "fix" it.

Generated by Rust PR Reviewer for issue #922 · sonnet46 2.5M · ◷